TCK tests for async invokers by Ladicek · Pull Request #706 · jakartaee/cdi-tck

Ladicek · 2026-04-13T14:20:54Z

Fixes #698

Ladicek · 2026-04-13T14:21:43Z

Draft because the TCK audit already contains changes from jakartaee/cdi#961. Otherwise this is ready for review and then merge.

Ladicek · 2026-04-16T11:43:38Z

Rebased. Since jakartaee/cdi#961 was merged, this is ready for review.

manovotn

We should also add a test for AfterDeploymentValidation.ensureAsyncHandlerExists()

Ladicek · 2026-04-20T11:28:50Z

We should also add a test for AfterDeploymentValidation.ensureAsyncHandlerExists()

If anything, we should add a copy of this PR for Portable Extensions. I don't see the need to add a test for this one method, without testing the rest of the infrastructure through PEs.

manovotn · 2026-04-20T12:00:48Z

We should also add a test for AfterDeploymentValidation.ensureAsyncHandlerExists()

If anything, we should add a copy of this PR for Portable Extensions. I don't see the need to add a test for this one method, without testing the rest of the infrastructure through PEs.

That would be even better for sure :)

manovotn · 2026-04-20T12:02:13Z

@Ladicek do you want to address the PE counterpart in this PR should we open a new one?
Because as far as BCEs are concerned, I am happy with this PR.

Ladicek · 2026-04-20T12:03:26Z

To be honest, I'd love someone else to do the PE part 😆

manovotn · 2026-04-20T14:59:48Z

To be honest, I'd love someone else to do the PE part 😆

I have added a PE variant of the three positive test cases - is that OK with you @Ladicek?

Ladicek · 2026-04-21T07:14:40Z

Just about perfect, thanks!

manovotn

LGTM.

Ladicek · 2026-04-27T14:19:38Z

Added a 2nd commit that contains TCK tests for the respecified async handlers (jakartaee/cdi#967). Before merging, the commits should be squashed.

Asked for a new review.

Ladicek · 2026-04-29T06:53:31Z

@manovotn I think I screwed up by force-pushing the new TCK, because that dropped your commit with PEs tests. Do you by any chance still have them locally? Thanks! :-)

Ladicek · 2026-05-04T08:24:00Z

Squashed and rebased.

@manovotn could you please take a look if you still have your PE tests locally? Sorry I dropped them.

manovotn · 2026-05-04T08:51:12Z

Squashed and rebased.

@manovotn could you please take a look if you still have your PE tests locally? Sorry I dropped them.

Pushed

manovotn

I went over the tests as I was implementing it in Weld and it LGTM 👍

manovotn · 2026-05-12T13:03:00Z

@Azquelt if you get a chance, could you please review this PR as well as #709?
I was holding these off until you get a chance to take a look.

Afterwards, I would love to work towards releasing Beta versions of both, CDI and TCKs; as per the release timeline for Jakarta, M4 is nearing.

Azquelt · 2026-05-12T13:11:32Z

Yeah, I will take a look

Azquelt

I noticed that some of the spec assertions are out of date and need fixed.

I also realised that with a parameter async handler the dependent beans may be destroyed before the method returns and I'm not sure if that's the behaviour we want (though that's a spec issue, not really related to these tests)

Azquelt · 2026-05-12T13:13:16Z

+
+    @Test
+    @SpecAssertion(section = Sections.USING_INVOKER_BUILDER_FULL, id = "a")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "ja")


ja should be la

Azquelt · 2026-05-12T13:13:58Z

+
+    @Test
+    @SpecAssertion(section = Sections.USING_INVOKER_BUILDER_FULL, id = "a")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "jb")


jb should be lb

Azquelt · 2026-05-12T13:14:32Z

+
+    @Test
+    @SpecAssertion(section = Sections.USING_INVOKER_BUILDER_FULL, id = "a")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "jc")


jc should be lc

Azquelt · 2026-05-12T14:28:56Z

+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "b")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "g")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "h")


Should be cf, hb, j I think?

cf, hb, i, j actually

Azquelt · 2026-05-12T14:29:29Z

+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "b")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "i")
+    public void testSyncThrow() {


cf and hc?

ha (synchronous completion) appears not to be tested in PE and neither is the transformReturnValue method.

cf and hc?

cf, hc, k actually

ha (synchronous completion) appears not to be tested in PE

Correct. I'm adding a test, because there's one for BCE.

and neither is the transformReturnValue method.

Need to take a look at this.

and neither is the transformReturnValue method.

Need to take a look at this.

This is actually more widespread: we don't have any test that would check transformReturnValue(). I'll try to add one (or two, actually -- for BCE and PE).

Azquelt · 2026-05-12T16:35:40Z

+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "b")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "g")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "h")
+    public void test() throws Exception {


be, gb and j

be, gb, i, j actually

Azquelt · 2026-05-12T16:37:38Z

+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "b")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "i")
+    public void testSyncThrow() {


be, gc and k

A test for synchronous successful completion is missing here too, and I'm also adding it.

Azquelt · 2026-05-12T17:08:36Z

+    @Test(dataProvider = ARQUILLIAN_DATA_PROVIDER)
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "cf")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "ha")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "i")
+    @SpecAssertion(section = Sections.INVOKER_ASYNCHRONOUS_METHODS, id = "j")
+    public void testSync(InvokerHolder invokers) throws Exception {
+        MyDependentBean.reset();
+
+        Invoker<MyBean, MyAsyncType<String>> hello = invokers.get("helloSync");
+        MyAsyncType<String> result = MyAsyncType.createSuspended();
+
+        assertEquals(MyDependentBean.destroyedCounter.get(), 0);
+
+        hello.invoke(null, new Object[] { null, result });
+
+        assertEquals(MyDependentBean.destroyedCounter.get(), 1);
+        assertTrue(result.isComplete());
+        assertEquals(result.getIfComplete(), "hello");
+    }


In this test we don't assert whether the dependent beans are destroyed before or after the method returns.

I think that, as currently specified, the beans should be destroyed as soon as async.resume is called inside the helloSync method, i.e. before the method returns, which is a little odd given that you could be destroying the bean that you called the method on and it's a bit difficult for the async handler to avoid that.

As the spec is currently written, I think this test is correct but we might want to consider deferring destruction until both complete is called and the method returns.

Very good observation! Let me think about that for a bit.

I think I addressed all comments except of this one. It is a little tricky.

Currently, the specification indeed requires that completion is signaled and hence CC is released before the method returns. This would be especially wrong if the target bean is @Dependent and its instance is looked up by the invoker.

I tried fixing the issue locally and there seems to be a relatively straightforward way, so I'll look at:

specifying that the CC is released after the method returns, even if completion was signaled before the method returned,

modifying the TCK.

Hopefully tomorrow.

Submitted a spec clarification: jakartaee/cdi#973

Will submit fixed tests to this PR later today.

Added one commit with a test. Turns out it's just one assert in an existing test, which seems nice :-)

A "two-way" async type signals completion through two different ways: - synchronous completion is signalled through the method's return value - asynchronous completion is signalled through the async parameter and the return value is a dummy placeholder This occurs in practice with Kotlin `suspend` functions, where if the function completes synchronously, it returns the value directly, and if it completes asynchronously, it signals completion through a parameter (and the return value is a placeholder). The tests in this commit are of course not written in Kotlin, but they are structured in the same way.

These tests ignore the return values of invokers, so the wrong return type has not been an issue. It is still wrong, though, and should be fixed.

Ladicek · 2026-05-13T14:07:50Z

Rebased and added 3 commits:

update Portable Extensions tests, should be squashed before merging
- fixes assertion IDs as reported by @Azquelt
add tests for async handlers of "two-way" async types
- adds missing tests that exercise transformReturnValue()
fix invoker return types in some tests
- fixes wrong Invoker return types; these were not used, so were not caught, but are wrong anyway and so should be fixed

…arget method is still running

Ladicek · 2026-05-14T13:13:39Z

This PR now includes a test of jakartaee/cdi#973

Ladicek · 2026-05-15T09:05:42Z

I intend to squash the commits here before merging, but will leave the PR in its current state for easier review.

Ladicek added this to the CDI 5.0 milestone Apr 13, 2026

Ladicek force-pushed the async-invokers branch from bea0465 to 0a5e6e5 Compare April 16, 2026 11:41

Ladicek marked this pull request as ready for review April 16, 2026 11:43

manovotn requested review from Azquelt and manovotn April 16, 2026 13:51

manovotn requested changes Apr 20, 2026

View reviewed changes

Comment thread ...g/jboss/cdi/tck/tests/invokers/lookup/dependent/async/invalid/MultipleAsyncHandlersTest.java Outdated

Comment thread ...oss/cdi/tck/tests/invokers/lookup/dependent/async/returntype/AsyncHandlerReturnTypeTest.java Outdated

Ladicek force-pushed the async-invokers branch from 0a5e6e5 to 33f3fbd Compare April 20, 2026 11:27

manovotn approved these changes Apr 21, 2026

View reviewed changes

Azquelt approved these changes Apr 21, 2026

View reviewed changes

Comment thread ...okers/lookup/dependent/async/paramtype/invalid/MultipleAsyncHandlersForOneParamTypeTest.java

Ladicek force-pushed the async-invokers branch from a1360c9 to 07cadee Compare April 27, 2026 14:16

Ladicek requested review from Azquelt and manovotn April 27, 2026 14:19

Ladicek force-pushed the async-invokers branch from 07cadee to 20078d9 Compare May 4, 2026 08:23

manovotn approved these changes May 4, 2026

View reviewed changes

Azquelt requested changes May 12, 2026

View reviewed changes

TCK tests for async invokers

336c1bd

manovotn and others added 4 commits May 13, 2026 13:32

Add Portable Extension variant of async invoker tests

ce20242

update Portable Extensions tests, should be squashed before merging

cee5452

fix invoker return types in some tests

9026bbc

These tests ignore the return values of invokers, so the wrong return type has not been an issue. It is still wrong, though, and should be fixed.

Ladicek force-pushed the async-invokers branch from 16e1684 to 9026bbc Compare May 13, 2026 14:05

add test that dependent looked up beans are not destroyed while the t…

e521d7c

…arget method is still running

update the audit file, to be squashed later

a508f20

manovotn mentioned this pull request May 14, 2026

Additional async handler fixes weld/core#3344

Merged

manovotn requested a review from Azquelt May 15, 2026 10:38

Conversation

Ladicek commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ladicek commented Apr 13, 2026

Uh oh!

Ladicek commented Apr 16, 2026

Uh oh!

manovotn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ladicek commented Apr 20, 2026

Uh oh!

manovotn commented Apr 20, 2026

Uh oh!

manovotn commented Apr 20, 2026

Uh oh!

Ladicek commented Apr 20, 2026

Uh oh!

manovotn commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ladicek commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manovotn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Ladicek commented Apr 27, 2026

Uh oh!

Ladicek commented Apr 29, 2026

Uh oh!

Ladicek commented May 4, 2026

Uh oh!

manovotn commented May 4, 2026

Uh oh!

manovotn left a comment

Choose a reason for hiding this comment

Uh oh!

manovotn commented May 12, 2026

Uh oh!

Azquelt commented May 12, 2026

Uh oh!

Azquelt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Ladicek commented Apr 13, 2026 •

edited

Loading

manovotn commented Apr 20, 2026 •

edited

Loading

Ladicek commented Apr 21, 2026 •

edited

Loading